Handle global disposable preload feature#686
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
==========================================
+ Coverage 70.59% 70.74% +0.15%
==========================================
Files 61 61
Lines 13306 13374 +68
==========================================
+ Hits 9393 9462 +69
+ Misses 3913 3912 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025070402-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061004-4.3&flavor=update
Failed tests10 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 12 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:No issues Remaining performance tests:No remaining performance tests |
|
There are some results in https://openqa.qubes-os.org/tests/142046 already. I'm not sure what is causing some of those failures yet. |
I don't see this PR included on OpenQA: |
ben-grande
force-pushed
the
preload-dispvm-dom0-feat
branch
2 times, most recently
from
3afd031 to
bb71368
Compare
|
Not ready, some corner cases when modifying either global or local qube feature. |
ben-grande
force-pushed
the
preload-dispvm-dom0-feat
branch
from
bb71368 to
3a10acd
Compare
qubes/vm/mix/dvmtemplate.py
Outdated
| global preload feature is set.""" | ||
| assert isinstance(self, qubes.vm.BaseVM) | ||
| if ( | ||
| self == getattr(self, "default_dispvm", None) |
There was a problem hiding this comment.
| self == getattr(self, "default_dispvm", None) | |
| self == getattr(self.app, "default_dispvm", None) |
otherwise you'll check VM's "default_dispvm" property, not the global one
ben-grande
force-pushed
the
preload-dispvm-dom0-feat
branch
4 times, most recently
from
c5cf131 to
f105a73
Compare
qubes/vm/mix/dvmtemplate.py
Outdated
| value = None | ||
| if ( | ||
| not force_local | ||
| and not getattr(self, "preload_max_ignore_global", False) |
There was a problem hiding this comment.
In what case this attribute is needed? When event handler sets it, the global default_dispvm is changed already, so the below condition would handle it already, no?
But if there is a case where it's needed, please add it properly in __init__ (mixin class can have __init__ too, just don't forget to call super()).
There was a problem hiding this comment.
Appears to be misconception that I presumed it would be needed and didn't test without it, plus it was before force_local.
There was a problem hiding this comment.
There doesn't seem to be a case where it is needed. I tested without it and it worked.
**app**:
property-(re)?set:default_dispvm:
* old_appvm = True
* new_appvm = False
**vm.adminvm**:
domain-feature-delete:preload-dispvm-max:
* True
domain-feature-set:preload-dispvm-max:
* False
qubes/tests/app.py
Outdated
| mock_new.assert_called_once() | ||
| self.assertEqual( | ||
| "domain-preload-dispvm-start", mock_new.call_args[0][0] | ||
| ) |
There was a problem hiding this comment.
| mock_new.assert_called_once() | |
| self.assertEqual( | |
| "domain-preload-dispvm-start", mock_new.call_args[0][0] | |
| ) | |
| mock_new.assert_called_once_with("domain-preload-dispvm-start") |
If there are other args that are not interesting for test, you can use unittest.mock.ANY as a placeholder.
Besides being easier to read, it's also easier to spot mistakes like too much copy&paste - see the one below checks mock_new again...
There was a problem hiding this comment.
Implemented with mock.ANY locally, much better.
qubes/tests/app.py
Outdated
| ) as mock_new: | ||
| self.app.default_dispvm = self.appvm_alt | ||
| mock_old.assert_not_called() | ||
| mock_new.assert_called_once() |
|
Now this needs a rebase |
ben-grande
force-pushed
the
preload-dispvm-dom0-feat
branch
3 times, most recently
from
4c9cbdb to
e45fee0
Compare
|
The global preload test is quite large for a single test but it has the advantage of having less code duplication, as later tests depends on previous tests plus there is a clear distinction between tests with logging. |
|
Liberated for OpenQA. |
|
ben-grande
force-pushed
the
preload-dispvm-dom0-feat
branch
from
e45fee0 to
befb836
Compare
qubes/tests/integ/dispvm.py
Outdated
| # each test function is applied. | ||
| if "_preload_" not in self._testMethodName: | ||
| self.app.default_dispvm = self.disp_base | ||
| self.cleanup_preload |
There was a problem hiding this comment.
| self.cleanup_preload | |
| self.cleanup_preload() |
?
There was a problem hiding this comment.
Hum, when running the test directly, it doesn't skip like it did on OpenQA. Fixing this mistake led to find another wrong function call.
qubes/tests/integ/dispvm.py
Outdated
| tasks = [self.app.domains[x].cleanup() for x in old_preload] | ||
| self.loop.run_until_complete(asyncio.gather(*tasks)) | ||
| self.disp_base.features["preload-dispvm-max"] = False | ||
| self.cleanup_preload |
ben-grande
force-pushed
the
preload-dispvm-dom0-feat
branch
from
befb836 to
e9151ea
Compare
|
Ran with the latest commit |
Finally :) |
3 participants
For: QubesOS/qubes-issues#1512
For: QubesOS/qubes-desktop-linux-manager#262